Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add unit tests for scansourcefiles actor #1190

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Add unit tests for scansourcefiles actor #1190

merged 1 commit into from
Apr 11, 2024

Conversation

tomasfratrik
Copy link
Member

Test if files are correctly tracked by actor. Create mocked existent/non-existent, modified/non-modified, rpm owned/not-owned files. Also test non-existent version files.

Jira: OAMG-10367

@github-actions GitHub Actions
Copy link

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp, e.g. from PR#42, use /packit test oamg/leapp#42
Note that first time contributors cannot run tests automatically - they will be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

  • /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
  • /packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

@tomasfratrik tomasfratrik marked this pull request as ready for review March 11, 2024 11:39
@tomasfratrik tomasfratrik changed the title [DRAFT] Add unit tests for scansourcefiles actor Add unit tests for scansourcefiles actor Mar 11, 2024
matejmatuska
matejmatuska requested changes Mar 11, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a common practice to first test the lower level functions as units and then mock them when testing functions that call them.

So in all of these 3 tests, you would just test the functionality on scansourcefiles.scan_file(). Then you would add a test that tests scansourcefiles.process() and mock scan_file() in it, because that function is already covered.

This would reduce the amount of mocks in the lower level tests and also allow you to run the test once per possible input (in this case a file). Then you could use @pytest.mark.parametrize() to run the test for multiple inputs (files). You can also specify the expected results there to avoid having specific asserts for each one of them.

You can take a look at https://github.com/oamg/leapp-repository/blob/master/repos/system_upgrade/el8toel9/actors/rocescanner/tests/unit_test_rocescanner.py for an example of such tests. First get_roce_nics_lines is properly tested and then it's mocked when testing process.

matejmatuska
matejmatuska requested changes Mar 22, 2024
View reviewed changes
Copy link
Member

@matejmatuska matejmatuska left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now looking a lot better. However some additional changes are requested.

I am sorry, I added 2 comments outside the review by accident.

repos/system_upgrade/common/actors/scansourcefiles/tests/unit_test_scansourcefiles.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/actors/scansourcefiles/tests/unit_test_scansourcefiles.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/actors/scansourcefiles/tests/unit_test_scansourcefiles.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/actors/scansourcefiles/tests/unit_test_scansourcefiles.py Outdated Show resolved Hide resolved
@tomasfratrik tomasfratrik force-pushed the scansourcefiles_test branch from f0527fc to f32fdca Compare March 27, 2024 15:19
@tomasfratrik
Copy link
Member Author

tomasfratrik commented Mar 27, 2024 •
edited
Loading

My notes:

  • I separated some values in pytest parametrize, because I think is now more readable (i can see it faster what is what, and it is not too much longer). - Now I see that testing farm won't allow me to do this, so I will fix this
  • I split test of function _get_rpm_name into 2 tests, 1 containing normal tests, and other one error. Now it could considered as 'redundant' (even though the code still differs), but I think it is more readable. Before there were basically multiple if statements that separated if it is error or not.
  • Removed one rpm test. After consideration, I realized it is not testing anything I haven't tested before
  • My thought process over test test_tracked_files, which tests process, was that, I will give 'you' (process function), tracked files, and major version, and you do the rest. Process should just take files from 'common' and 'major_version' but I "don't know that", so I let it do whatever it wants to, and then compare. And mainly compare produced message.

I will squash commits, and write proper commit message after approved, or when it is finalized.

@pirat89
Copy link
Member

pirat89 commented Mar 28, 2024

/packit build

pirat89
pirat89 previously approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's much better! lgtm!

@pirat89
Copy link
Member

pirat89 commented Mar 28, 2024

/packit rebuild-failed

@fernflower fernflower requested a review from matejmatuska March 28, 2024 12:37
@pirat89
Copy link
Member

pirat89 commented Mar 28, 2024

/packit rebuild-failed

@pirat89
Copy link
Member

pirat89 commented Mar 28, 2024

/packit build

@tomasfratrik tomasfratrik force-pushed the scansourcefiles_test branch from d207f7d to 697452a Compare April 2, 2024 08:14
@tomasfratrik
Copy link
Member Author

When splitting _get_rpm_name I forgot to check warnmsg, should be fixed now.

@tomasfratrik tomasfratrik force-pushed the scansourcefiles_test branch from 697452a to 7da7cc7 Compare April 2, 2024 11:47
@pirat89
Copy link
Member

pirat89 commented Apr 3, 2024

@fernflower seems that automatic building is broken by general.

@pirat89
Copy link
Member

pirat89 commented Apr 11, 2024

/packit build

@matejmatuska matejmatuska merged commit 3066cad into oamg:master Apr 11, 2024
15 checks passed
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Jul 2, 2024
yuravk pushed a commit to yuravk/leapp-repository that referenced this pull request Aug 16, 2024
@tomasfratrik @yuravk
Add unit tests for scansourcefiles actor (oamg#1190)
ef75ecd 
Jira: OAMG-10367
(cherry picked from commit 3066cad)
yuravk pushed a commit to yuravk/leapp-repository that referenced this pull request Aug 20, 2024
@tomasfratrik @yuravk
Add unit tests for scansourcefiles actor (oamg#1190)
6127345 
Jira: OAMG-10367
(cherry picked from commit 3066cad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejmatuska matejmatuska matejmatuska approved these changes

@pirat89 pirat89 pirat89 approved these changes

Assignees
No one assigned
Labels
changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomasfratrik @pirat89 @matejmatuska